src/main/java/org/example/visacasemanagementsystem/user/entity/User.java (1)

25-29: Consider DB-level nullable = false on the new username and password columns.

@NotBlank is bean-validation only and runs during save lifecycle; it does not produce a NOT NULL DDL constraint. If validation is ever bypassed (native query, direct SQL, disabled validator), you could end up with null usernames/passwords — and UserDetailsServiceImpl / Spring Security will behave poorly with either. Mirror the existing style used on fullName:

🛡️ Proposed fix

-    `@NotBlank` `@Column`(unique = true)
-    private String username;
-
-    `@NotBlank` `@Column`
-    private String password;
+    `@NotBlank` `@Column`(nullable = false, unique = true)
+    private String username;
+
+    `@NotBlank` `@Column`(nullable = false)
+    private String password;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/visacasemanagementsystem/user/entity/User.java`
around lines 25 - 29, The new User entity fields username and password use
`@NotBlank` (bean validation) but lack DB-level NOT NULL; update the User class to
add nullable = false to the `@Column` annotations on the username and password
fields (mirror the existing fullName style) so the database schema enforces
non-null values and prevents null usernames/passwords from being
persisted/bypassing validation.

src/main/java/org/example/visacasemanagementsystem/user/UserAuthorization.java (1)

6-11: Overriding toString() on the enum has wide-reaching side effects.

Every caller that prints or serializes a UserAuthorization — logs, Thymeleaf expressions like ${user.userAuthorization}, JSON serialization (Jackson uses toString() for enums only when WRITE_ENUMS_USING_TO_STRING is enabled, but many custom toString() calls in User.toString() and similar do use it), and any .toString() invoked on a UserAuthorization value — will now emit ROLE_USER/ROLE_ADMIN/ROLE_SYSADMIN instead of USER/ADMIN/SYSADMIN. This can silently break views, log output, and any other string-based consumers.

@Enumerated(EnumType.STRING) and Enum.valueOf(...) rely on name() (not toString()), so persistence still works — but the broader behavioral change is easy to forget.

Consider exposing a dedicated method instead and constructing the authority explicitly in UserPrincipal:

♻️ Proposed refactor

 public enum UserAuthorization {
     USER,
     ADMIN,
-    SYSADMIN;
-
-    `@Override`
-    public String toString() {
-        return "ROLE_" + this.name();
-    }
+    SYSADMIN;
+
+    public String asAuthority() {
+        return "ROLE_" + name();
+    }
 }

And in UserPrincipal.getAuthorities():

return Collections.singleton(new SimpleGrantedAuthority(user.getUserAuthorization().asAuthority()));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/visacasemanagementsystem/user/UserAuthorization.java`
around lines 6 - 11, Overriding toString() on the UserAuthorization enum
introduces wide-reaching breakage; remove the toString() override from
UserAuthorization and add a dedicated accessor (e.g., asAuthority() or
toRoleString()) that returns "ROLE_" + name(), then update the authority
construction in UserPrincipal.getAuthorities() (use new
SimpleGrantedAuthority(user.getUserAuthorization().asAuthority())) so all
logging/serialization still sees the original enum names while security gets the
ROLE_ prefixed string.

src/main/java/org/example/visacasemanagementsystem/config/SecurityConfig.java (1)

41-45: defaultSuccessUrl("/dashboard", true) forces the user back to /dashboard even after they tried to access a deep link.

Passing true as the second arg means "always redirect to this URL on success", so the saved-request redirect flow (return the user to the page they originally attempted) is disabled. For a dashboard-heavy app this may be intentional, but if deep-linking is ever desired, switch to false (or omit) and let Spring's SavedRequestAwareAuthenticationSuccessHandler do the right thing.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/visacasemanagementsystem/config/SecurityConfig.java`
around lines 41 - 45, The current formLogin configuration in SecurityConfig uses
defaultSuccessUrl("/dashboard", true) which always redirects to /dashboard and
prevents returning users to saved deep links; change the call in the
SecurityConfig class (the formLogin(...) block where defaultSuccessUrl is
configured) to use defaultSuccessUrl("/dashboard", false) or remove the boolean
so SavedRequestAwareAuthenticationSuccessHandler can perform saved-request
redirects instead.

src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java (1)

104-141: Consolidate role‑check helpers and rename the validateSysAdmin overload.

Two issues in this cluster:

1. validateSysAdmin(Long requesterId) (line 104) and validateSysAdmin(UserPrincipal principal) (line 123) share a name but have completely different semantics — one loads a user by id from the DB and checks the entity field, the other reads authorities off the authenticated principal. Overload resolution at the call site hides that divergence; give them distinct names (e.g., requireSysAdminById vs requireSysAdmin) so reviewers can tell which trust source is used.
2. validateProfileAccess, validateSysAdmin(UserPrincipal), and validateAdmin all repeat the same principal.getAuthorities().stream().anyMatch(...) pattern with role strings. Extract a small hasRole(UserPrincipal, String) helper (or a constants class for ROLE_SYSADMIN/ROLE_ADMIN) to remove the duplication and keep role names in one place.

♻️ Sketch

+    private static boolean hasRole(UserPrincipal principal, String role) {
+        return principal.getAuthorities().stream()
+                .anyMatch(a -> Objects.equals(a.getAuthority(), role));
+    }
+
     public void validateProfileAccess(UserPrincipal principal, Long userId) {
         boolean isOwnProfile = principal.getUserId().equals(userId);
-        boolean isSysAdmin = principal.getAuthorities().stream()
-                .anyMatch(a -> Objects.equals(a.getAuthority(), "ROLE_SYSADMIN"));
-        if (!isOwnProfile && !isSysAdmin) {
+        if (!isOwnProfile && !hasRole(principal, "ROLE_SYSADMIN")) {
             throw new UnauthorizedException("You do not have permission to edit this profile.");
         }
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java`
around lines 104 - 141, Rename the DB-backed validateSysAdmin(Long requesterId)
to requireSysAdminById and rename the principal-based
validateSysAdmin(UserPrincipal principal) to requireSysAdmin so callers clearly
indicate which trust source is used; then extract a single helper
hasRole(UserPrincipal principal, String role) (and optionally central
ROLE_SYSADMIN / ROLE_ADMIN constants) and refactor validateProfileAccess,
requireSysAdmin, and validateAdmin to call hasRole(...) instead of repeating
principal.getAuthorities().stream().anyMatch(...), updating all call sites
accordingly.

src/main/java/org/example/visacasemanagementsystem/user/security/UserPrincipal.java (2)

42-60: Drop the redundant UserDetails default overrides.

isAccountNonExpired, isAccountNonLocked, isCredentialsNonExpired, and isEnabled all just delegate to UserDetails.super.*. These overrides add noise and invite confusion about whether custom account‑state logic exists. Remove them until real behavior is needed (e.g., backed by enabled/locked fields on User).

♻️ Proposed cleanup

-    `@Override`
-    public boolean isAccountNonExpired() {
-        return UserDetails.super.isAccountNonExpired();
-    }
-
-    `@Override`
-    public boolean isAccountNonLocked() {
-        return UserDetails.super.isAccountNonLocked();
-    }
-
-    `@Override`
-    public boolean isCredentialsNonExpired() {
-        return UserDetails.super.isCredentialsNonExpired();
-    }
-
-    `@Override`
-    public boolean isEnabled() {
-        return UserDetails.super.isEnabled();
-    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/visacasemanagementsystem/user/security/UserPrincipal.java`
around lines 42 - 60, The four redundant overrides in
UserPrincipal—isAccountNonExpired, isAccountNonLocked, isCredentialsNonExpired,
and isEnabled—simply delegate to UserDetails.super and should be removed to
reduce noise; delete these methods from the UserPrincipal class (leaving other
UserDetails methods intact) and only reintroduce custom implementations later if
you implement real account-state fields (e.g., enabled/locked) on the User
model.

---

20-24: Avoid deriving authority strings from UserAuthorization.toString().

Using toString() as a business‑critical serialization format is fragile — toString() is conventionally for debugging/logging, and any future change (e.g., adding a display name, adjusting log formatting) will silently break authorization. Expose an explicit accessor on UserAuthorization (or build the string here) so the contract is intentional and searchable.

♻️ Proposed refactor

-    `@Override`
-    `@NullMarked`
-    public Collection<? extends GrantedAuthority> getAuthorities() {
-        return Collections.singleton(new SimpleGrantedAuthority(user.getUserAuthorization().toString()));
-    }
+    `@Override`
+    `@NullMarked`
+    public Collection<? extends GrantedAuthority> getAuthorities() {
+        return Collections.singleton(
+                new SimpleGrantedAuthority("ROLE_" + user.getUserAuthorization().name()));
+    }

Then the custom UserAuthorization#toString() override can be dropped, leaving toString() to its default purpose.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/visacasemanagementsystem/user/security/UserPrincipal.java`
around lines 20 - 24, The getAuthorities() implementation in UserPrincipal
should not rely on UserAuthorization.toString(); instead add an explicit
accessor (e.g., UserAuthorization#getAuthority or `#getRoleName` or use an enum
`#name`()) on the UserAuthorization type and call that here; change
UserPrincipal.getAuthorities() to create the SimpleGrantedAuthority from that
explicit accessor (e.g., new
SimpleGrantedAuthority(user.getUserAuthorization().getAuthority())), and
update/introduce the accessor in the UserAuthorization class so the authority
string is a stable, documented contract (you can later remove any custom
toString() override).

src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java (1)

73-88: viewProfile re‑computes authorization flags already evaluated by validateProfileAccess.

validateProfileAccess at line 79 already performs the isOwnProfile || isSysAdmin check (throwing on failure), and lines 81‑86 recompute essentially the same predicate to set canEdit. That's two sources of truth for the same rule — if the policy ever changes (e.g., ADMIN can also edit), it'll have to be edited in both places.

Consider either returning the decision from UserService (e.g., boolean canEditProfile(UserPrincipal, Long)) or replacing the anyMatch literal with the same hasRole helper suggested in UserService.java, so "ROLE_SYSADMIN" isn't hard‑coded in two files.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java`
around lines 73 - 88, The viewProfile method currently duplicates authorization
logic from userService.validateProfileAccess; replace the local recomputation
with a single call to the service so there is one source of truth: remove the
isOwnProfile/isSysAdmin anyMatch logic and instead call a new or existing
service method (e.g., userService.canEditProfile(principal, userId)) or use the
UserService.hasRole helper to determine edit permission, then set
model.addAttribute("canEdit", userService.canEditProfile(principal, userId));
keep validateProfileAccess for throwing on unauthorized access but derive the
view flag from the service method to avoid duplicated hard‑coded
"ROLE_SYSADMIN".

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/main/java/org/example/visacasemanagementsystem/user/security/UserPrincipal.java (3)

31-34: @Nullable on getPassword() contradicts the entity contract.

User.password is declared @NotBlank @column(nullable = false), so getPassword() should never return null for a persisted user. Marking it @Nullable here will force callers (and any null-analysis tooling) to handle a case that cannot occur, and it's inconsistent with the @NullMarked treatment applied to getUsername() which has the same entity guarantees. Consider marking this @NullMarked too (or simply leaving it unannotated if the class/package is already null-marked).

🧹 Proposed change

-    `@Override`
-    public `@Nullable` String getPassword() {
-        return user.getPassword();
-    }
+    `@Override`
+    `@NullMarked`
+    public String getPassword() {
+        return user.getPassword();
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/visacasemanagementsystem/user/security/UserPrincipal.java`
around lines 31 - 34, Remove the incorrect `@Nullable` annotation on
UserPrincipal.getPassword(), since User.getPassword() is annotated
`@NotBlank/`@Column(nullable = false) and thus cannot be null; update the method
in class UserPrincipal to either mark it as null-safe consistent with
getUsername() (e.g., use `@NullMarked/`@NonNull as per the project's nullability
convention) or leave it unannotated if the package/class is already null-marked
so callers and static analysis treat getPassword() as non-null.

---

20-24: Guard against a null userAuthorization.

getAuthorities() dereferences user.getUserAuthorization() unconditionally. While the column is declared @NotNull on the entity, a partially-populated User (e.g., constructed in tests, or loaded before the field is set) will trigger an NPE inside Spring Security's authentication pipeline, which surfaces as an opaque 500 rather than a clear auth failure. Consider returning an empty authority collection (or throwing a descriptive exception) when the authorization is missing.

🛡️ Proposed defensive check

-    public Collection<? extends GrantedAuthority> getAuthorities() {
-        return Collections.singleton(new SimpleGrantedAuthority(user.getUserAuthorization().asAuthority()));
-    }
+    public Collection<? extends GrantedAuthority> getAuthorities() {
+        var authorization = user.getUserAuthorization();
+        if (authorization == null) {
+            return Collections.emptyList();
+        }
+        return Collections.singleton(new SimpleGrantedAuthority(authorization.asAuthority()));
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/visacasemanagementsystem/user/security/UserPrincipal.java`
around lines 20 - 24, getAuthorities() currently dereferences
user.getUserAuthorization() and can NPE; update UserPrincipal.getAuthorities()
to defensively check user.getUserAuthorization() for null and if null return
Collections.emptyList() (or alternatively throw a descriptive
IllegalStateException) instead of constructing a SimpleGrantedAuthority; ensure
the check references the existing user field and the SimpleGrantedAuthority
creation logic so you only create authorities when user.getUserAuthorization()
is non-null.

---

1-45: Move @NullMarked to the class level instead of annotating individual methods.

The current pattern of sprinkling @NullMarked on individual methods is verbose and inconsistent—getAuthorities() and getUsername() are annotated, but getFullName() and getUserId() are not, despite having identical nullness semantics. Per JSpecify guidance, @NullMarked is intended for class or package declarations, with @Nullable used to opt out specific members (in this case, getPassword()). This approach eliminates per-method noise and ensures uniform null-safety contracts across the class.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/visacasemanagementsystem/user/security/UserPrincipal.java`
around lines 1 - 45, Add `@NullMarked` to the UserPrincipal class declaration and
remove the per-method `@NullMarked` annotations on getAuthorities() and
getUsername(); keep the `@Nullable` on getPassword() to opt-out of the class-level
nullness and remove annotations from other methods like getFullName() and
getUserId() so the class-level nullness applies consistently for UserPrincipal.